Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add ucmerced staging hub with profileList for Python and R images #3218

Merged
merged 5 commits into from
Oct 4, 2023
Merged

Add ucmerced staging hub with profileList for Python and R images #3218

merged 5 commits into from
Oct 4, 2023

Conversation

GeorgianaElena
Copy link
Member

@GeorgianaElena GeorgianaElena commented Oct 4, 2023 •
edited
Loading

@GeorgianaElena GeorgianaElena requested a review from a team as a code owner October 4, 2023 06:37
@github-actions
Copy link

github-actions bot commented Oct 4, 2023 •
edited
Loading

Merging this PR will trigger the following deployment actions.

Support and Staging deployments

Cloud Provider Cluster Name Upgrade Support? Reason for Support Redeploy Upgrade Staging? Reason for Staging Redeploy
gcp 2i2c No Yes Following helm chart values files were modified: enc-ucmerced-staging.secret.values.yaml, ucmerced-staging.values.yaml, ucmerced-common.values.yaml

Production deployments

Cloud Provider Cluster Name Hub Name Reason for Redeploy
gcp 2i2c ucmerced Following helm chart values files were modified: ucmerced.values.yaml, ucmerced-common.values.yaml

sgibson91
sgibson91 reviewed Oct 4, 2023
View reviewed changes
Copy link
Member

@sgibson91 sgibson91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth having a ucmerced-common.values.yaml file to reduce repetition, e.g., homepage config?

@GeorgianaElena
Copy link
Member Author

Thanks @consideRatio and @sgibson91 for the review. I've updated the PR based on the feedback. Also, I've manually deployed this and is runnitng at https://staging.ucmerced.2i2c.cloud/ What do you think?

consideRatio
consideRatio approved these changes Oct 4, 2023
View reviewed changes
Copy link
Contributor

@consideRatio consideRatio left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the adding a staging hub as a way to help the many users of ucmerced test the experience of choosing one of these images while migrating away from the one we have maintained.

Can we add a note in cluster.yaml next to this staging hub that this is to be considered an exception, and that once they want the same experience in the prod hub, we remove this staging hub?

@sgibson91
Copy link
Member

@GeorgianaElena what was the motivation for only using profile lists on staging? I actually don't see this mentioned in the original issue

@consideRatio consideRatio mentioned this pull request Oct 4, 2023
Closed
@consideRatio
Copy link
Contributor

@sgibson91 as a strategy to help them move away from using the old 2i2c maintained image, this relates to #2674 and #2583

@sgibson91
Copy link
Member

Right, I just didn't think that was why the original issue was opened. I think it was opened because I tanked prod while trying to debug something right before a class 😅

@GeorgianaElena
Copy link
Member Author

Sorry for not linking the main issue @sgibson91 (#3188)

I have now also added a comment in cluster.yaml about this as well.

@GeorgianaElena GeorgianaElena merged commit 0da8fdd into 2i2c-org:master Oct 4, 2023
@GeorgianaElena GeorgianaElena deleted the uc-merced-staging branch October 4, 2023 15:23
@github-actions
Copy link

github-actions bot commented Oct 4, 2023

🎉🎉🎉🎉

Monitor the deployment of the hubs here 👉 https://github.com/2i2c-org/infrastructure/actions/runs/6408123254

@consideRatio
Copy link
Contributor

consideRatio commented Oct 4, 2023 •
edited
Loading

Oops I understood this initially as a "let this hub be deployed first to catch issues via our automation", but its more like a production hub that is deployed side by side with the real production ucmerced hub - used as a staging hub for the end users.

image

I think long term if we do this again, it could be good to call this "trial hub" or similar to avoid that kind of confusion. That would then also have implied that its a short term hub.

@sgibson91
Copy link
Member

sgibson91 commented Oct 4, 2023 •
edited
Loading

I'm surprised it didn't launch as a staging job given this logic

infrastructure/deployer/helm_upgrade_decision.py

Lines 315 to 322 in e9a7ac7

# Separate the jobs for hubs with "staging" in their name (including "dask-staging")
# from those without staging in their name
staging_hub_jobs = [
job for job in all_hub_matrix_jobs if "staging" in job["hub_name"]
]
prod_hub_matrix_jobs = [
job for job in all_hub_matrix_jobs if "staging" not in job["hub_name"]
]

Edited to add: Actually, I think what the workflow file can't (yet) handle is multiple staging hubs on a given cluster. We make an explicit exception for "staging" and "dask-staging" on the 2i2c cluster.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sgibson91 sgibson91 sgibson91 left review comments

@consideRatio consideRatio consideRatio approved these changes

Assignees

@GeorgianaElena GeorgianaElena

Labels
None yet
Projects
No open projects
Sprint Board
Status: Done 🎉
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add a staging hub for UCMerced
3 participants
@GeorgianaElena @sgibson91 @consideRatio